-
Notifications
You must be signed in to change notification settings - Fork 23
feat(agent): implement DVC remote exec detached mode #1567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Let maintainers know that an action is required on their side
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a detached execution mode for the agent's process execution functionality, allowing fire-and-forget process launches without IO redirection or active session management.
Key changes:
- Added
run_detached()method to spawn processes that run independently without IO monitoring - Refactored
WinApiProcessCtxto remove the storedio_notification_txfield, passing it as a parameter instead - Implemented console window hiding for all spawned processes (not just detached ones)
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| devolutions-session/src/dvc/task.rs | Added detached mode handling in four message processors (exec_process, exec_batch, exec_winps, exec_pwsh), checking is_detached() flag and calling run_detached() instead of normal execution flow |
| devolutions-session/src/dvc/process.rs | Refactored WinApiProcessCtx to pass io_notification_tx as parameter; added run_impl() and run_detached() methods; implemented console window hiding via SW_HIDE flag |
| devolutions-session/Cargo.toml | Updated now-proto-pdu dependency to use git branch feat/exec-detached instead of crates.io version |
| Cargo.lock | Updated lockfile with new now-proto-pdu version from git and resolved transitive dependency versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@pacmancoder I've opened a new pull request, #1568, to work on those changes. Once the pull request is ready, I'll request review from you. |
e535c79 to
92b4dd5
Compare
bc9511c to
1a880f3
Compare
|
CI fails because of an unrelated test failure |
CBenoit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
For the failed test, you can now rebase on top of master where this is fixed.
Before merging, please apply the PR guidelines: https://github.com/Devolutions/devolutions-gateway/blob/master/AGENTS.md#pull-request-guidelines
I’ll let you wrap up and merge 🙂
1a880f3 to
50d89fb
Compare
|
Hi @pacmancoder Small heads-up to avoid the markdown link in the footer: The changelog generator consume everything on the right of
Resulting into this: [ARC-411](https://devolutions.atlassian.net/browse/[ARC-411](https://devolutions.atlassian.net/browse/ARC-411)) |
Adds fire-and-forget remote execution via the now proto DVC.
Previously, all execution types (except Run) waited for the process exit code and tracked the execution session, but this behavior is not always what the user expects. This PR changes that and adds an option to specify if fire and forget mode is needed (return result right after process is spawned).
Issue: ARC-411